From ce5a9dfcddaa9351523dd865a51bae4535072e37 Mon Sep 17 00:00:00 2001 From: Debian Qt/KDE Maintainers Date: Thu, 27 Nov 2025 15:54:31 +0100 Subject: [PATCH] CVE-2024-39936 Origin: https://github.com/qt/qtbase/commit/bb1006b789f4ed0183b6ca668a45f75243879cb6 Reviewed-by: Sylvain Beucler Last-Update: 2025-11-26 From bb1006b789f4ed0183b6ca668a45f75243879cb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Tue, 25 Jun 2024 17:09:35 +0200 Subject: [PATCH] HTTP2: Delay any communication until encrypted() can be responded to We have the encrypted() signal that lets users do extra checks on the established connection. It is emitted as BlockingQueued, so the HTTP thread stalls until it is done emitting. Users can potentially call abort() on the QNetworkReply at that point, which is passed as a Queued call back to the HTTP thread. That means that any currently queued signal emission will be processed before the abort() call is processed. In the case of HTTP2 it is a little special since it is multiplexed and the code is built to start requests as they are available. This means that, while the code worked fine for HTTP1, since one connection only has one request, it is not working for HTTP2, since we try to send more requests in-between the encrypted() signal and the abort() call. This patch changes the code to delay any communication until the encrypted() signal has been emitted and processed, for HTTP2 only. It's done by adding a few booleans, both to know that we have to return early and so we can keep track of what events arose and what we need to resume once enough time has passed that any abort() call must have been processed. Conflict resolution: adapt to Qt 5 code supporting SPDY as well. Fixes: QTBUG-126610 Pick-to: 5.12 Change-Id: Ic25a600c278203256e35f541026f34a8783235ae Reviewed-by: Marc Mutz Reviewed-by: Volker Hilsheimer (cherry picked from commit b1e75376cc3adfc7da5502a277dfe9711f3e0536) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit 0fb43e4395da34d561814242a0186999e4956e28) (cherry picked from commit 2b1e36e183ce75c224305c7a94457b92f7a5cf58) Reviewed-by: Timur Pocheptsov (cherry picked from commit 2b3048b35c4b37bfbfe38b8bde999715806bd7b8) (cherry picked from commit db8bd4ea27e53753875ec71feed9c5b562b713eb) Gbp-Pq: Name CVE-2024-39936.diff --- src/network/access/qhttp2protocolhandler.cpp | 6 +-- .../access/qhttpnetworkconnectionchannel.cpp | 46 ++++++++++++++++++- .../access/qhttpnetworkconnectionchannel_p.h | 6 +++ tests/auto/network/access/http2/tst_http2.cpp | 44 ++++++++++++++++++ 4 files changed, 97 insertions(+), 5 deletions(-) diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index 4c9e72216..a4afe858b 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -365,12 +365,12 @@ bool QHttp2ProtocolHandler::sendRequest() } } - if (!prefaceSent && !sendClientPreface()) - return false; - if (!requests.size()) return true; + if (!prefaceSent && !sendClientPreface()) + return false; + m_channel->state = QHttpNetworkConnectionChannel::WritingState; // Check what was promised/pushed, maybe we do not have to send a request // and have a response already? diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp index d355eafd7..72771612a 100644 --- a/src/network/access/qhttpnetworkconnectionchannel.cpp +++ b/src/network/access/qhttpnetworkconnectionchannel.cpp @@ -255,6 +255,10 @@ void QHttpNetworkConnectionChannel::abort() bool QHttpNetworkConnectionChannel::sendRequest() { Q_ASSERT(!protocolHandler.isNull()); + if (waitingForPotentialAbort) { + needInvokeSendRequest = true; + return false; // this return value is unused + } return protocolHandler->sendRequest(); } @@ -267,21 +271,28 @@ bool QHttpNetworkConnectionChannel::sendRequest() void QHttpNetworkConnectionChannel::sendRequestDelayed() { QMetaObject::invokeMethod(this, [this] { - Q_ASSERT(!protocolHandler.isNull()); if (reply) - protocolHandler->sendRequest(); + sendRequest(); }, Qt::ConnectionType::QueuedConnection); } void QHttpNetworkConnectionChannel::_q_receiveReply() { Q_ASSERT(!protocolHandler.isNull()); + if (waitingForPotentialAbort) { + needInvokeReceiveReply = true; + return; + } protocolHandler->_q_receiveReply(); } void QHttpNetworkConnectionChannel::_q_readyRead() { Q_ASSERT(!protocolHandler.isNull()); + if (waitingForPotentialAbort) { + needInvokeReadyRead = true; + return; + } protocolHandler->_q_readyRead(); } @@ -1285,7 +1296,18 @@ void QHttpNetworkConnectionChannel::_q_encrypted() // Similar to HTTP/1.1 counterpart below: const auto &pairs = spdyRequestsToSend.values(); // (request, reply) const auto &pair = pairs.first(); + waitingForPotentialAbort = true; emit pair.second->encrypted(); + + // We don't send or handle any received data until any effects from + // emitting encrypted() have been processed. This is necessary + // because the user may have called abort(). We may also abort the + // whole connection if the request has been aborted and there is + // no more requests to send. + QMetaObject::invokeMethod(this, + &QHttpNetworkConnectionChannel::checkAndResumeCommunication, + Qt::QueuedConnection); + // In case our peer has sent us its settings (window size, max concurrent streams etc.) // let's give _q_receiveReply a chance to read them first ('invokeMethod', QueuedConnection). QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection); @@ -1303,6 +1325,26 @@ void QHttpNetworkConnectionChannel::_q_encrypted() } } +void QHttpNetworkConnectionChannel::checkAndResumeCommunication() +{ + Q_ASSERT(connection->connectionType() > QHttpNetworkConnection::ConnectionTypeHTTP); + + // Because HTTP/2 requires that we send a SETTINGS frame as the first thing we do, and respond + // to a SETTINGS frame with an ACK, we need to delay any handling until we can ensure that any + // effects from emitting encrypted() have been processed. + // This function is called after encrypted() was emitted, so check for changes. + + if (!reply && spdyRequestsToSend.isEmpty()) + abort(); + waitingForPotentialAbort = false; + if (needInvokeReadyRead) + _q_readyRead(); + if (needInvokeReceiveReply) + _q_receiveReply(); + if (needInvokeSendRequest) + sendRequest(); +} + void QHttpNetworkConnectionChannel::requeueSpdyRequests() { QList spdyPairs = spdyRequestsToSend.values(); diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h index d8ac3979d..eac444649 100644 --- a/src/network/access/qhttpnetworkconnectionchannel_p.h +++ b/src/network/access/qhttpnetworkconnectionchannel_p.h @@ -107,6 +107,10 @@ public: QAbstractSocket *socket; bool ssl; bool isInitialized; + bool waitingForPotentialAbort = false; + bool needInvokeReceiveReply = false; + bool needInvokeReadyRead = false; + bool needInvokeSendRequest = false; ChannelState state; QHttpNetworkRequest request; // current request, only used for HTTP QHttpNetworkReply *reply; // current reply for this request, only used for HTTP @@ -187,6 +191,8 @@ public: void closeAndResendCurrentRequest(); void resendCurrentRequest(); + void checkAndResumeCommunication(); + bool isSocketBusy() const; bool isSocketWriting() const; bool isSocketWaiting() const; diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp index 95a979021..e69a159db 100644 --- a/tests/auto/network/access/http2/tst_http2.cpp +++ b/tests/auto/network/access/http2/tst_http2.cpp @@ -111,6 +111,8 @@ private slots: void connectToHost(); void maxFrameSize(); + void abortOnEncrypted(); + protected slots: // Slots to listen to our in-process server: void serverStarted(quint16 port); @@ -776,6 +778,48 @@ void tst_Http2::maxFrameSize() QVERIFY(serverGotSettingsACK); } +void tst_Http2::abortOnEncrypted() +{ +#if !QT_CONFIG(ssl) + QSKIP("TLS support is needed for this test"); +#else + clearHTTP2State(); + serverPort = 0; + + ServerPtr targetServer(newServer(defaultServerSettings, H2Type::h2Direct)); + + QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection); + runEventLoop(); + + nRequests = 1; + nSentRequests = 0; + + const auto url = requestUrl(H2Type::h2Direct); + QNetworkRequest request(url); + request.setAttribute(QNetworkRequest::Http2DirectAttribute, true); + + std::unique_ptr reply{manager->get(request)}; + reply->ignoreSslErrors(); + connect(reply.get(), &QNetworkReply::encrypted, reply.get(), [reply = reply.get()](){ + reply->abort(); + }); + connect(reply.get(), &QNetworkReply::errorOccurred, this, &tst_Http2::replyFinishedWithError); + + runEventLoop(); + STOP_ON_FAILURE + + QCOMPARE(nRequests, 0); + QCOMPARE(reply->error(), QNetworkReply::OperationCanceledError); + + const bool res = QTest::qWaitFor( + [this, server = targetServer.get()]() { + return serverGotSettingsACK || prefaceOK || nSentRequests > 0; + }, + 500); + QVERIFY(!res); +#endif // QT_CONFIG(ssl) +} + void tst_Http2::serverStarted(quint16 port) { serverPort = port; -- 2.30.2